Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fully port InverseCancellation to Rust #13013

Merged
merged 6 commits into from
Sep 6, 2024

Conversation

mtreinish
Copy link
Member

@mtreinish mtreinish commented Aug 21, 2024

Summary

This commit builds off of #12959 and the other data model in Rust
infrastructure and migrates the InverseCancellation pass to
operate fully in Rust. The full path of the transpiler pass now never
leaves Rust until it has finished modifying the DAGCircuit. There is
still some python interaction necessary to handle parts of the data
model that are still in Python, mainly for handling parameter
expressions. But otherwise the entirety of the pass
operates in rust now.

This is just a first pass at the migration here, it moves the pass to
use loops in rust. The next steps here are to look at operating
the pass in parallel. There is no data dependency between the
optimizations being done for different inverse gates/pairs so we should
be able to the throughput of the pass by leveraging multithreading to
handle each inverse option in parallel. This commit does not attempt
this though, because of the Python dependency and also the data
structures around gates and the dag aren't really setup for
multithreading yet and there likely will need to be some work to
support that.

Details and comments

This PR is based on top of #12959 and as such github shows the entire contents of #12959 in addition to the contents of this PR. To see the contents of this PR you can look at HEAD on this branch, or just look at:
00c7e10
rebased

Fixes #12271
Part of #12208

TODO:

@mtreinish mtreinish added performance Rust This PR or issue is related to Rust code in the repository mod: transpiler Issues and PRs related to Transpiler labels Aug 21, 2024
@mtreinish mtreinish added this to the 1.3 beta milestone Aug 21, 2024
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @enavarro51
  • @Qiskit/terra-core
  • @kevinhartman
  • @levbishop
  • @mtreinish
  • @nkanazawa1989

@mtreinish mtreinish force-pushed the rust-inverse-cancellation branch from 98e6159 to 7614689 Compare August 22, 2024 16:54
@coveralls
Copy link

coveralls commented Aug 22, 2024

Pull Request Test Coverage Report for Build 10745716091

Details

  • 181 of 189 (95.77%) changed or added relevant lines in 5 files are covered.
  • 9 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.02%) to 89.158%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/circuit/src/dag_circuit.rs 39 41 95.12%
crates/accelerate/src/inverse_cancellation.rs 138 144 95.83%
Files with Coverage Reduction New Missed Lines %
qiskit/synthesis/two_qubit/xx_decompose/decomposer.py 1 95.42%
crates/qasm2/src/lex.rs 2 93.23%
crates/qasm2/src/parse.rs 6 97.61%
Totals Coverage Status
Change from base Build 10745615622: 0.02%
Covered Lines: 72883
Relevant Lines: 81746

💛 - Coveralls

mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Aug 23, 2024
This commit builds off of Qiskit#13013 and the other data model in Rust
infrastructure and migrates the InverseCancellation pass to
operate fully in Rust. The full path of the transpiler pass now never
leaves Rust until it has finished modifying the DAGCircuit. There is
still some python interaction necessary to handle parts of the data
model that are still in Python, mainly for creating `UnitaryGate`
instances and `ParameterExpression` for global phase. But otherwise
the entirety of the pass operates in rust now.

This is just a first pass at the migration here, it moves the pass to
use loops in rust. The next steps here are to look at operating
the pass in parallel. There is no data dependency between the
optimizations being done for different gates so we should be able to
increase the throughput of the pass by leveraging multithreading to
handle each gate in parallel. This commit does not attempt
this though, because of the Python dependency and also the data
structures around gates and the dag aren't really setup for
multithreading yet and there likely will need to be some work to
support that.

Part of Qiskit#12208
@mtreinish mtreinish force-pushed the rust-inverse-cancellation branch from 7614689 to 00c7e10 Compare August 25, 2024 11:34
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Aug 25, 2024
This commit builds off of Qiskit#13013 and the other data model in Rust
infrastructure and migrates the InverseCancellation pass to
operate fully in Rust. The full path of the transpiler pass now never
leaves Rust until it has finished modifying the DAGCircuit. There is
still some python interaction necessary to handle parts of the data
model that are still in Python, mainly for creating `UnitaryGate`
instances and `ParameterExpression` for global phase. But otherwise
the entirety of the pass operates in rust now.

This is just a first pass at the migration here, it moves the pass to
use loops in rust. The next steps here are to look at operating
the pass in parallel. There is no data dependency between the
optimizations being done for different gates so we should be able to
increase the throughput of the pass by leveraging multithreading to
handle each gate in parallel. This commit does not attempt
this though, because of the Python dependency and also the data
structures around gates and the dag aren't really setup for
multithreading yet and there likely will need to be some work to
support that.

Part of Qiskit#12208
This commit builds off of Qiskit#12959 and the other data model in Rust
infrastructure and migrates the InverseCancellation pass to
operate fully in Rust. The full path of the transpiler pass now never
leaves Rust until it has finished modifying the DAGCircuit. There is
still some python interaction necessary to handle parts of the data
model that are still in Python, mainly for handling parameter
expressions. But otherwise the entirety of the pass
operates in rust now.

This is just a first pass at the migration here, it moves the pass to
use loops in rust. The next steps here are to look at operating
the pass in parallel. There is no data dependency between the
optimizations being done for different inverse gates/pairs so we should
be able to the throughput of the pass by leveraging multithreading to
handle each inverse option in parallel. This commit does not attempt
this though, because of the Python dependency and also the data
structures around gates and the dag aren't really setup for
multithreading yet and there likely will need to be some work to
support that.

Fixes Qiskit#12271
Part of Qiskit#12208
@mtreinish mtreinish force-pushed the rust-inverse-cancellation branch from 00c7e10 to 170148c Compare August 30, 2024 21:16
@mtreinish mtreinish changed the title [WIP] Fully port InverseCancellation to Rust Fully port InverseCancellation to Rust Aug 30, 2024
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Aug 30, 2024
This commit builds off of Qiskit#13013 and the other data model in Rust
infrastructure and migrates the InverseCancellation pass to
operate fully in Rust. The full path of the transpiler pass now never
leaves Rust until it has finished modifying the DAGCircuit. There is
still some python interaction necessary to handle parts of the data
model that are still in Python, mainly for creating `UnitaryGate`
instances and `ParameterExpression` for global phase. But otherwise
the entirety of the pass operates in rust now.

This is just a first pass at the migration here, it moves the pass to
use loops in rust. The next steps here are to look at operating
the pass in parallel. There is no data dependency between the
optimizations being done for different gates so we should be able to
increase the throughput of the pass by leveraging multithreading to
handle each gate in parallel. This commit does not attempt
this though, because of the Python dependency and also the data
structures around gates and the dag aren't really setup for
multithreading yet and there likely will need to be some work to
support that.

Part of Qiskit#12208
Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far!

Let me know what you think about the comments (they're mostly minor and not blocking).

crates/accelerate/src/inverse_cancellation.rs Outdated Show resolved Hide resolved
}
let mut collect_set: HashSet<String> = HashSet::with_capacity(1);
collect_set.insert(gate.operation.name().to_string());
let gate_runs: Vec<Vec<NodeIndex>> = dag.collect_runs(collect_set).unwrap().collect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like DAGCircuit::collect_runs returns an Option only because it directly returns the result of the Rustworkx core function, which uses None to represent a cycle in the graph (...which is questionable lol).

Perhaps it is beyond the scope of this PR, but it might be nice if you can change DAGCircuit::collect_runs to unwrap that option internally. If it fails, it should be on DAGCircuit for getting itself into an invalid state.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm fine with making this change to collect_runs() but I'd say lets save this for a follow-up PR so we can do it in isolation.

crates/accelerate/src/inverse_cancellation.rs Outdated Show resolved Hide resolved
} else {
panic!("Not an op node")
};
if inst.qubits != next_qargs {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! I think this is the first example I've seen of the intern IDs saving us some time :)

crates/accelerate/src/inverse_cancellation.rs Outdated Show resolved Hide resolved
.and_modify(|count| *count += value)
.or_insert(*value);
}
let circuit_to_dag = imports::CIRCUIT_TO_DAG.get_bound(py);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's worth adding a count_ops method to CircuitData, and calling that directly instead of converting to a DAGCircuit first.

I'm not saying that we should necessarily bother maintaining op counts in CircuitData like we do in a DAGCircuit, but computing on the fly should still be faster than recursively converting everything to a DAGCircuit.

Not necessary for this PR, but I wouldn't be opposed to doing it here either.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's funny you mention this, that was added last week: 5fc1635

but the implementation there is governed by the api of the python space method and not how I would implement it for performance in the transpiler.

kevinhartman
kevinhartman previously approved these changes Sep 6, 2024
Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Thanks for the changes! We can fix the result type of DAGCircuit::collect_runs in a follow-up PR.

@kevinhartman kevinhartman added this pull request to the merge queue Sep 6, 2024
Merged via the queue into Qiskit:main with commit 94ba19e Sep 6, 2024
15 checks passed
@mtreinish mtreinish deleted the rust-inverse-cancellation branch September 7, 2024 00:09
github-merge-queue bot pushed a commit that referenced this pull request Sep 9, 2024
* Fully port Split2QUnitaries to rust

This commit builds off of #13013 and the other data model in Rust
infrastructure and migrates the InverseCancellation pass to
operate fully in Rust. The full path of the transpiler pass now never
leaves Rust until it has finished modifying the DAGCircuit. There is
still some python interaction necessary to handle parts of the data
model that are still in Python, mainly for creating `UnitaryGate`
instances and `ParameterExpression` for global phase. But otherwise
the entirety of the pass operates in rust now.

This is just a first pass at the migration here, it moves the pass to
use loops in rust. The next steps here are to look at operating
the pass in parallel. There is no data dependency between the
optimizations being done for different gates so we should be able to
increase the throughput of the pass by leveraging multithreading to
handle each gate in parallel. This commit does not attempt
this though, because of the Python dependency and also the data
structures around gates and the dag aren't really setup for
multithreading yet and there likely will need to be some work to
support that.

Part of #12208

* Update pass logic with changes from #13095

Some of the logic inside the Split2QUnitaries pass was updated in a
recently merged PR. This commit makes those changes so the rust
implementation matches the current state of the previous python version.

* Use op_nodes() instead of topological_op_nodes()

* Use Fn trait instead of FnMut for callback

We don't need the callback to be mutable currently so relax the trait to
just be `Fn` instead of `FnMut`. If we have a need for a mutable
environment callback in the future we can change this easily enough
without any issues.

* Avoid extra edge operations in replace_on_incoming_qubits

* Rename function
@raynelfss raynelfss added the Changelog: None Do not include in changelog label Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog mod: transpiler Issues and PRs related to Transpiler performance Rust This PR or issue is related to Rust code in the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port InverseCancellation to Rust
5 participants